Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
Pull request overview
Adds Xbox Gen3 controller detection (via MAC OUI matching) to override devices whose advertised name equals their MAC address, enabling an immediate/meaningful UI name during discovery.
Changes:
- Add Xbox Gen3 OUI constants/list and a helper (
btrCore_IsXboxGen3Gamepad) to detect matching devices by MAC prefix. - Update
btrCore_BTDeviceStatusUpdateCbto replace the device name with"Xbox Wireless Controller"when the name equals the address and the OUI matches.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Xbox Gen3 exception: force immediate UI update with a temporary name */ | ||
| if (btrCore_IsDevNameSameAsAddress(&FoundDevice) && | ||
| btrCore_IsXboxGen3Gamepad(FoundDevice.pcDeviceAddress)) { | ||
| errno_t rc; | ||
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| BTRCORELOG_INFO("Gen3 detected by OUI; forcing UI update with temporary name for %s\n",FoundDevice.pcDeviceAddress); |
There was a problem hiding this comment.
This change introduces new behavioral logic (Xbox Gen3 OUI detection that mutates pcName to force an immediate UI update), but the existing unit tests for btrCore_BTDeviceStatusUpdateCb don’t appear to exercise this branch. Please add a test case where deviceInfo.pcName initially equals deviceInfo.pcAddress and the address matches one of the new OUIs, and assert that the callback updates the name as expected.
| /* Xbox Gen3 exception: force immediate UI update with a temporary name */ | ||
| if (btrCore_IsDevNameSameAsAddress(&FoundDevice) && | ||
| btrCore_IsXboxGen3Gamepad(FoundDevice.pcDeviceAddress)) { |
There was a problem hiding this comment.
PR description says this is for "Debug purpose for coverity", but the diff adds product behavior (new Xbox Gen3 OUI list + forcing device name updates). Please update the PR description to reflect the actual functional change and expected user impact, or gate this logic behind a debug/diagnostic flag if it’s intended to be temporary.
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); |
There was a problem hiding this comment.
In the new Xbox Gen3 block, strcpy_s is called with BD_NAME_LEN as the destination size for both FoundDevice.pcDeviceName and apstBTDeviceInfo->pcName. FoundDevice.pcDeviceName is sized BD_NAME_LEN + 1 and apstBTDeviceInfo->pcName is BT_MAX_STR_LEN, so passing BD_NAME_LEN is an incorrect/needlessly small bound and can turn future string changes into runtime-constraint failures. Prefer passing the actual destination buffer size (e.g., sizeof(FoundDevice.pcDeviceName) / sizeof(apstBTDeviceInfo->pcName) or the correct constant including the NUL).
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); | |
| ERR_CHK(rc); | |
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); | |
| rc = strcpy_s(FoundDevice.pcDeviceName, sizeof(FoundDevice.pcDeviceName), "Xbox Wireless Controller"); | |
| ERR_CHK(rc); | |
| rc = strcpy_s(apstBTDeviceInfo->pcName, sizeof(apstBTDeviceInfo->pcName), "Xbox Wireless Controller"); |
| /* Xbox Gen3 exception: force immediate UI update with a temporary name */ | ||
| if (btrCore_IsDevNameSameAsAddress(&FoundDevice) && | ||
| btrCore_IsXboxGen3Gamepad(FoundDevice.pcDeviceAddress)) { | ||
| errno_t rc; | ||
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| BTRCORELOG_INFO("Gen3 detected by OUI; forcing UI update with temporary name for %s\n",FoundDevice.pcDeviceAddress); | ||
| } | ||
|
|
There was a problem hiding this comment.
The indentation/bracing in the new Xbox Gen3 exception block is inconsistent with the surrounding code (mixed tab/space at the comment line and misaligned closing brace). Please reformat this block to match the file’s existing indentation so it’s easier to read and avoids diffs caused by whitespace-only changes later.
| /* Xbox Gen3 exception: force immediate UI update with a temporary name */ | |
| if (btrCore_IsDevNameSameAsAddress(&FoundDevice) && | |
| btrCore_IsXboxGen3Gamepad(FoundDevice.pcDeviceAddress)) { | |
| errno_t rc; | |
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); | |
| ERR_CHK(rc); | |
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); | |
| ERR_CHK(rc); | |
| BTRCORELOG_INFO("Gen3 detected by OUI; forcing UI update with temporary name for %s\n",FoundDevice.pcDeviceAddress); | |
| } | |
| /* Xbox Gen3 exception: force immediate UI update with a temporary name */ | |
| if (btrCore_IsDevNameSameAsAddress(&FoundDevice) && | |
| btrCore_IsXboxGen3Gamepad(FoundDevice.pcDeviceAddress)) { | |
| errno_t rc; | |
| rc = strcpy_s(FoundDevice.pcDeviceName, BD_NAME_LEN, "Xbox Wireless Controller"); | |
| ERR_CHK(rc); | |
| rc = strcpy_s(apstBTDeviceInfo->pcName, BD_NAME_LEN, "Xbox Wireless Controller"); | |
| ERR_CHK(rc); | |
| BTRCORELOG_INFO("Gen3 detected by OUI; forcing UI update with temporary name for %s\n", FoundDevice.pcDeviceAddress); | |
| } |
| if (btrCore_IsDevNameSameAsAddress(&FoundDevice) && | ||
| btrCore_IsXboxGen3Gamepad(FoundDevice.pcDeviceAddress)) { | ||
| errno_t rc; | ||
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); |
There was a problem hiding this comment.
Coverity Issue - Array compared against 0
Comparing an array to null is not useful: ""Xbox Wireless Controller" != NULL", since the test will always evaluate as true.
Medium Impact, CWE-398
NO_EFFECT
How to fix
Was ""Xbox Wireless Controller"" formerly declared as a pointer?
| rc = strcpy_s(FoundDevice.pcDeviceName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); |
There was a problem hiding this comment.
Coverity Issue - Array compared against 0
Comparing an array to null is not useful: ""Xbox Wireless Controller" != NULL", since the test will always evaluate as true.
Medium Impact, CWE-398
NO_EFFECT
How to fix
Was ""Xbox Wireless Controller"" formerly declared as a pointer?
| rc = strcpy_s(apstBTDeviceInfo->pcName,BD_NAME_LEN,"Xbox Wireless Controller"); | ||
| ERR_CHK(rc); | ||
|
|
||
| BTRCORELOG_INFO("Gen3 detected by OUI; forcing UI update with temporary name for %s\n",FoundDevice.pcDeviceAddress); |
There was a problem hiding this comment.
Coverity Issue - String not null terminated
Passing unterminated string "FoundDevice.pcDeviceAddress" to "fprintf", which expects a null-terminated string.
High Impact, CWE-170
STRING_NULL
Reason for change: Debug purpose for coverity
Test Procedure: Check ticket description
Risks: Medium
Priority: P1